catch errors on platform #573
Conversation
Codecov Report
@@ Coverage Diff @@
## master #573 +/- ##
==========================================
+ Coverage 85.86% 86.78% +0.91%
==========================================
Files 6 6
Lines 276 280 +4
Branches 42 43 +1
==========================================
+ Hits 237 243 +6
+ Misses 27 26 -1
+ Partials 12 11 -1
Continue to review full report at Codecov.
|
src/WebClient.spec.js
Outdated
| @@ -191,6 +191,19 @@ describe('WebClient', function () { | |||
| }); | |||
| }); | |||
|
|
|||
| it('should return platform error if 200 request is not ok', function () { | |||
There was a problem hiding this comment.
can we rephrase the description of this test case a bit? how about should fail with platform errors when the API response is an error?
src/WebClient.spec.js
Outdated
| return client.apiCall('method') | ||
| .catch((error) => { | ||
| assert.propertyVal(error, 'code', 'slackclient_platform_error'); | ||
| assert.propertyVal(error.data, 'ok', false); |
There was a problem hiding this comment.
this works since you have indirectly checked the truthiness of error in the preceding line. but i think this ca be improved as assert.nestedPropertyVal(error, 'data.ok', false); since then the order of the assertions wouldn't be significant.
There was a problem hiding this comment.
taken care of in latest commit; changed line 202 to assert.nestedPropertyVal
src/WebClient.spec.js
Outdated
| it('should return platform error if 200 request is not ok', function () { | ||
| const scope = nock('https://slack.com') | ||
| .post(/api/) | ||
| .reply(200, { ok: false }); |
There was a problem hiding this comment.
this response isn't sufficient to test the code. there should be an error property in the response, the way the actual API would provide one.
There was a problem hiding this comment.
taken care of in latest commit; added the property error: 'bad error' to line 197's test response object
src/WebClient.spec.js
Outdated
| .catch((error) => { | ||
| assert.propertyVal(error, 'code', 'slackclient_platform_error'); | ||
| assert.propertyVal(error.data, 'ok', false); | ||
| scope.done(); |
There was a problem hiding this comment.
we're missing an assertion on error.data.error, which should be equal to the error property in the response (as commented above).
There was a problem hiding this comment.
taken care of in latest commit; along with error: 'bad error' to line 197, also added assert.nestedPropertyVal(error, 'data.error', 'bad error') to inner function of the catch
src/WebClient.spec.js
Outdated
| @@ -191,15 +191,16 @@ describe('WebClient', function () { | |||
| }); | |||
| }); | |||
|
|
|||
| it('should return platform error if 200 request is not ok', function () { | |||
| it.only('should fail with platform errors when the API response is an error', function () { | |||
There was a problem hiding this comment.
ah, lets get rid of this .only()
Summary
This commit is solely to catch platform errors upon receiving a 200 request that is actually not ok. Test is also included.
Requirements (place an
xin each[ ])